Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(v2): add remark-admonitions to @docusaurus/preset-classic #2224

Merged
merged 7 commits into from
Feb 8, 2020
Merged

feat(v2): add remark-admonitions to @docusaurus/preset-classic #2224

merged 7 commits into from
Feb 8, 2020

Conversation

elviswolcott
Copy link
Contributor

Motivation

I wrote a remark plugin for admonitions (remark-admonitions) and mentioned it over in #1238. @yangshun suggested I add it to @docusaurus/preset-classic so I did.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

I tested it in two ways.

  1. Creating a new site with the modified version of @docusaurus/init using both templates
  2. Creating a new site from scratch using the modified version @docusaurus/preset-classic

screenshot

Related PRs

I haven't added anything to the docs yet as I'm not sure where it would fall under. Right now I'm thinking any docs would go in https://v2.docusaurus.io/docs/markdown-features if admonitions need to be mentioned.

@facebook-github-bot
Copy link
Contributor

Hi elviswolcott! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@elviswolcott elviswolcott changed the title Add remark-admonitions to @docusaurus/preset-classic Add remark-admonitions to @docusaurus/preset-classic Jan 17, 2020
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jan 17, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jan 17, 2020

Deploy preview for docusaurus-2 ready!

Built with commit bc60549

https://deploy-preview-2224--docusaurus-2.netlify.com

@elviswolcott elviswolcott changed the title Add remark-admonitions to @docusaurus/preset-classic feat(v2): add remark-admonitions to @docusaurus/preset-classic Jan 17, 2020
Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely addition! We need mandatory documentation with syntax descriptions and configuration examples.

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@elviswolcott
Copy link
Contributor Author

Where would be the appropriate place to add docs?

It seems like https://v2.docusaurus.io/docs/presets/#docusauruspreset-classic would be the right place to mention the config fields (i.e. options.docs.admonitions and options.blog.admonitions.

It seems like the most appropriate place to mention the added syntax would be https://v2.docusaurus.io/docs/markdown-features but I'm not sure if that makes sense given that it's only available if you're using @docusaurus/preset-classic.

@lex111
Copy link
Contributor

lex111 commented Jan 18, 2020

but I'm not sure if that makes sense given that it's only available if you're using @docusaurus/preset-classic.

On this page we have a corresponding note:

Note: All the following content assumes you are using docusaurus-preset-classic.

Therefore go ahead this way.

@elviswolcott
Copy link
Contributor Author

elviswolcott commented Jan 18, 2020

I don't know how I missed that! I'll get the docs added.

@elviswolcott
Copy link
Contributor Author

updated docs using admonitions

If only there was a way to make that more obvious 🤔

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall! So grateful for this! Left some suggestions for your consideration. If you think I can make the changes myself I'd be glad to do so. We can add more docs later but the docs you've added looks great.

This is important
:::

:::caution
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO caution and warning are synonyms. I think it's more conventional for yellow to be caution/warning and red for danger.

See VuePress' version - https://vuepress.vuejs.org/guide/markdown.html#custom-containers

Copy link
Contributor Author

@elviswolcott elviswolcott Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already have an alias of danger -> caution. I went with those words so that it is fully backwards compatible with remarkable-admonitions so that anyone upgrading their site will get the expected results.

I based the ordering of caution warning danger on the guidelines provided here favoloso/remarkable-admonitions#4

I can switch around the aliases (i.e. make it warning and danger with caution -> warning) for consistency with Infima over backwards compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yangshun which approach would you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just a nit comment, it's fine as-is 😄

packages/docusaurus-preset-classic/src/index.js Outdated Show resolved Hide resolved
packages/docusaurus-preset-classic/src/index.js Outdated Show resolved Hide resolved
website/docs/presets.md Show resolved Hide resolved
@lex111 lex111 added the pr: new feature This PR adds a new API or behavior. label Jan 24, 2020
@elviswolcott elviswolcott requested a review from yangshun February 5, 2020 20:30
Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks @elviswolcott!

@yangshun yangshun merged commit 0fa080c into facebook:master Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants